Speed up project discovery #242
Conversation
|
I will have a look at this tonight. |
|
Note that 3 core-it tests were failing for me, but they also fail with Maven 3.6.0, so they are probably environment dependent. Are there any specific requirements for these tests? I'd really like to see a fully green build :) |
|
Project discovery on the project took ~5min before, now down to ~2.5min. Still a long way to go, but a good improvement :) |
|
Nevermind, I got it - We can't remember the All integration tests are passing now. |
maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
Show resolved
Hide resolved
maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
Show resolved
Hide resolved
| if ( tok.hasMoreTokens() ) | ||
| { | ||
| qualifier = tok.nextToken(); | ||
| fallback = Pattern.compile( "\\d+" ).matcher( qualifier ).matches(); |
There was a problem hiding this comment.
Why not use the char array approach as before?
There was a problem hiding this comment.
Because it wasn't a hotspot in my tests. I can do it for consistency, but I can't prove that it is a problem :)
There was a problem hiding this comment.
Why not, if you know that this works better that with regex.
There was a problem hiding this comment.
I generally only change things if I can measure they are a problem. Otherwise one can quickly get into a situation where things get worse or the code gets less readable without a benefit.
There was a problem hiding this comment.
I've adjusted it, as in this case it also improves readability.
maven-artifact/src/main/java/org/apache/maven/artifact/versioning/DefaultArtifactVersion.java
Show resolved
Hide resolved
| return null; | ||
| } | ||
| long longValue = Long.parseLong( s ); | ||
| if ( longValue > Integer.MAX_VALUE ) |
There was a problem hiding this comment.
Again, change in semantics with long. Dislike on this one. Why all the hassle, why not use NumberUtils from Commons Lang 3?
There was a problem hiding this comment.
Because it throws an exception internally, which is what we're trying to avoid in the first place here. We don't want to throw exceptions on common code paths.
Also, we're not changing semantics here, we are still returning an int. Going through long is only done for the very common case of big numbers that would fail with a NumberFormatException on Integer.parseInt
There was a problem hiding this comment.
+1 to @oehme, however it is probably worth adding a comment that explains this (the reason behind tryParseInt method and the reason behind parseLong).
| @Override | ||
| public int hashCode() | ||
| { | ||
| return value; |
There was a problem hiding this comment.
Why not use Objects#hashCode() on this?
| @@ -211,6 +235,30 @@ public int compareTo( Item item ) | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals( Object o ) | |||
There was a problem hiding this comment.
Why not use Objects#equals() on this?
| } | ||
|
|
||
| @Override | ||
| public int hashCode() |
There was a problem hiding this comment.
Why not use Objects#hashCode() on this?
There was a problem hiding this comment.
What for? It's a single object, not a list.
| @@ -272,6 +320,29 @@ public int compareTo( Item item ) | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals( Object o ) | |||
| @@ -384,6 +455,29 @@ public int compareTo( Item item ) | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean equals( Object o ) | |||
Why is this overhead necessary? I wouldn't know what to write in the ticket besides "look at the commit message" ;)
I did, each change removes a similar chunk of the build time. They are all very much worth it. I can send you the original and after profiler snapshot if you'd like. |
This isn't overhead. Every change needs to be documented as a ticket. They go into the release notes. We don't expect users to read the log to understand what has been changed. |
oehme
force-pushed
the
master
branch
3 times, most recently
from
186586c to
f4b6e48
Compare
Use a simple list of allowed characters instead of a regex.
oehme
force-pushed
the
master
branch
2 times, most recently
from
dca4f99 to
2b34a4e
Compare
By not allocating the canonical representation for equals/hashcode, but instead using the items we already have. This saves both time and memory. I left the canonical field around for testing purposes.
Use if-statements instead of exception-based control flow. Throwing exceptions is very expensive and should not be used for normal flow.
Otherwise we have to go through the whole sisu engine again, which is very slow, because it does a linear scan.
|
I've created the issues and addressed your comments above, please have another look. |
|
Here are two profiles, with the optimized methods colored in pink and their total contribution to CPU time shown at the bottom: Before: After: As you can see, optimizing |
if you follow the instructions https://maven.apache.org/core-its/core-it-suite/, it should work easily on any configuration (as it works on ASF Jenkins and on many Maven developers). But there may be weak ITs that are fragile against something in your environment: what are the failing ITs and your configuration, please? |
|
another side: I'd like to be able to reproduce your tests and profiling. |
|
@rfscholte, @britter and I are meeting about this next week. How about you join in as well and I can walk you through some of the methodology? |
|
I had to re-launch the build on ASF Jenkins for MNG-6632 2 times before it went ok https://builds.apache.org/view/M-R/view/Maven/job/maven-box/job/maven/job/MNG-6632/ I reviewed also the commits: everything looks perfect to me I'll approve these changes on GitHub and finish the approval on dev list: IMHO, we'll need to find a simpler process for the future... |
|
We'll probably need to fix https://issues.apache.org/jira/browse/MNG-6643 afterwards, i.e. ComparableVersion should not depend on any third party libraries so it can easily be executed from commandline. |
Can you tell more accurate when you like to meet? |
|
@khmarbaise the meeting was yesterday afternoon - @britter will post a summary on the mailing list soon. |
|
@khmarbaise the summary is here: https://lists.apache.org/thread.html/240bec41fd91580027661a167dbde7aa001ff03aa0b20ef779f927ce@%3Cdev.maven.apache.org%3E If we do this kind of calls again I will make sure more people get a chance to join. It was kind of improvised over the easter weekend. Sorry! |
|
@hboutemy Only one integration test still fails locally for me (an Thanks for approving the changes. I was wondering one thing: Is using Guava allowed in the core module? It's already a transitive dependency of Maven and it could simplify some of the code I wrote for this PR. |
|
PR merged, and additional commit for explanation added thanks a lot |
|
Thanks @hboutemy! Can you comment on my earlier question? It would help me make my future contributions as concise as possible:
|
|
I prefer not to use guava, we've seen way too many backwords compatibility issues with this library. IIRC it is used by Sisu, I hope that's the only one. |
| } | ||
| } | ||
|
|
||
| private boolean isValidId( String id ) |
There was a problem hiding this comment.
Should this be static? (in order to prevent accidental access to instance fields)
I profiled the project discovery phase of a large (2000 module) Maven build today, because it was taking several minutes before it actually started executing goals. I found some low-level hotspots that this PR fixes. It shaves off about 2.5min of the startup time of this build. See the individual commit messages for more details. There are still other hotspots left, but they are in the plexus-interpolation project, for which I'll open a separate PR.
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MNG-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MNG-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.